Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash of extension when proxy settings address has wrong format #1046

Merged

Conversation

angelozerr
Copy link
Contributor

@angelozerr angelozerr commented Dec 11, 2024

This PR should fix the following problem:

2024-11-22 14:07:45.651 [error] Activating extension redhat.vscode-xml failed due to an error:
2024-11-22 14:07:45.651 [error] TypeError: Cannot read properties of null (reading '1')
	at t.getProxySettings (/Users/userid/.vscode/extensions/redhat.vscode-xml-0.27.2024111708-darwin-arm64/dist/extension.js:2:354116)

reported at #154 (comment)

I think to reproduce the problem, proxy address have a wrong format (don't start with https://)

A simple test that I did in debug is:

const HOST_AND_PORT_EXTRACTOR = /https?:\/\/([^:/]+)(?::([0-9]+))?/;
HOST_AND_PORT_EXTRACTOR .exec('foo") // -> returns undefined

@angelozerr angelozerr self-assigned this Dec 11, 2024
@angelozerr angelozerr requested a review from datho7561 December 11, 2024 14:28
@angelozerr angelozerr added this to the 0.27.3 milestone Dec 11, 2024
Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks pretty good so far.

I think, if we want to support something like 192.168.1.23:9090 as the proxy, we can use this regex:

(?:https?:\/\/)?([^:/]+)(?::([0-9]+))?

which just wraps the https:// part in a non-capturing group and makes it optional.

From what I can tell, we don't use the http:// or https:// part of the url anyways, so it shouldn't matter if it's left out.

@angelozerr
Copy link
Contributor Author

I have tried the const HOST_AND_PORT_EXTRACTOR = (?:https?://)?([^:/]+)(?::([0-9]+))?;

but it seems it is not a valid regexp

@datho7561
Copy link
Contributor

ahh I think you need to surround it in //:

const HOST_AND_PORT_EXTRACTOR = /(?:https?://)?([^:/]+)(?::([0-9]+))?/;

@datho7561
Copy link
Contributor

diff --git a/src/settings/proxySettings.ts b/src/settings/proxySettings.ts
index 3cbcf38..6ee7d0c 100644
--- a/src/settings/proxySettings.ts
+++ b/src/settings/proxySettings.ts
@@ -152,7 +152,7 @@ export function jvmArgsContainsProxySettings(jvmArgs: string): boolean {
   );
 }
 
-const HOST_AND_PORT_EXTRACTOR = /https?:\/\/([^:/]+)(?::([0-9]+))?/;
+const HOST_AND_PORT_EXTRACTOR = /(?:https?:\/\/)?([^:/]+)(?::([0-9]+))?/;
 
 const JVM_PROXY_HOST = 'http.proxyHost';
 const JVM_PROXY_PORT = 'http.proxyPort';

@angelozerr angelozerr force-pushed the getproxySettings_undefined branch from 6875f50 to 32508ef Compare December 12, 2024 17:36
@angelozerr
Copy link
Contributor Author

Thanks @datho7561 !

It seems it is working pretty well.

Please merge the PR if you think it is good.

@datho7561 datho7561 merged commit f666928 into redhat-developer:main Dec 12, 2024
1 check passed
@datho7561
Copy link
Contributor

Thanks for fixing this, Angelo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants